Add KMS options to catalogs create CLI #3330
Conversation
Following up on apache#2802
| current_kms_key=options_get(Arguments.KMS_KEY_CURRENT), | ||
| allowed_kms_keys=options_get(Arguments.KMS_KEY_ALLOWED), |
There was a problem hiding this comment.
does just setting current_kms_key automatically adds the allowed_kms_keys ?
There was a problem hiding this comment.
Currently "current" is the same as "allowed"... However, I believe only "current" needs write access... but that's in Polaris java code... it does not affect CLI.
| ) | ||
| PATH_STYLE_ACCESS = "(Only for S3) Whether to use path-style-access for S3" | ||
| KMS_KEY_CURRENT = ( | ||
| "(Only for AWS S3) The AWS KMS key ARN to be used for encrypting new S3 data" |
There was a problem hiding this comment.
I am assuming this is required because we need to use this key to encrypt metadata.json ? as when we are vending creds we don't know which snapshot the client will be reading so we vend creds for all or we just give decrypt creds for allowed key and encrypt | decrypt creds for current keys ?
There was a problem hiding this comment.
Polaris does not use KMS keys directly. It only generates AWS policies that allow those keys to be used on the AWS side when S3 requests are made. But, yes, the current key is used for writing new data. Zero or more additional keys are also allowed to be used because they might be required for dealing with old files.
There was a problem hiding this comment.
Polaris does not use KMS keys directly.
wouldn't we be needing this for encrypting / decrypting metadata.json ?
additional keys are also allowed to be used because they might be required for dealing with old files
I agree with additional keys but my question was why would Polaris vends creds for old kms keys for encrypting, files are immutable, so old keys should be vended for decrypt, similarly new key should have encrypt / decrypt.
Do we vend creds for encryption and decryting for all key in our sts policy ?
There was a problem hiding this comment.
my question was why would Polaris vends creds for old kms keys for encrypting, [...]
Currently it does. However, this is beyond the scope of current PR (CLI). It's about the actual java code from #2802 :)
Normally, I'd think "additional" keys should get only decryption rights, but this may be tricky from the manual key rotation perspective.
| "(Only for S3) Indicates that Polaris should not use STS (e.g. if STS is not available)" | ||
| ) | ||
| PATH_STYLE_ACCESS = "(Only for S3) Whether to use path-style-access for S3" | ||
| KMS_KEY_CURRENT = ( |
There was a problem hiding this comment.
As these are optional and only for AWS, we may want to update client/python/apache_polaris/cli/command/catalogs.py as well for the function _has_aws_storage_info(). Here is a reference: https://github.com/apache/polaris/pull/3305/files#diff-a3e865c2a57514f7f505c706a3af70a5ac90b712f96656b513cdbfcee20c031eL181
There was a problem hiding this comment.
good point - updated
github-project-automation
bot
moved this from PRs In Progress
to Ready to merge
in Basic Kanban Board
singhpk234
left a comment
singhpk234
left a comment
There was a problem hiding this comment.
LGTM thanks @dimas-b !
github-project-automation
bot
moved this from Ready to merge
to Done
in Basic Kanban Board
* Disable renovate bot for openapi generator cli (apache#3306) * Fix openapi-generator-cli version in build system * Fix openapi-generator-cli version in build system * Build: Ensure reproducible .properties files (apache#3089) This is a safety net in case Properties are generated anywhere during the build. * Generate release vote e-mail as Github Step Summary (apache#3150) * Improve error handling in quickstart setup script (apache#3288) Add error detection and validation to API calls in the quickstart docker-compose setup with clear error messages for easier debugging. * (doc): Fix tools doc and add tool doc for mcp server (apache#3311) * chore(deps): update actions/stale digest to a21a081 (apache#3326) * Add cancel release candidate workflow (apache#3321) This commit fixes apache#3080 * fix(deps): update quarkus platform and group to v3.30.5 (apache#3329) * chore(deps): update actions/checkout digest to 8e8c483 (apache#3319) * fix(deps): update dependency org.agrona:agrona to v2.4.0 (apache#3333) * (feat): Helm: add priority class name support for helm (apache#3310) * Add priority class name support for helm * Update changelog * Update default port for metrics from 8282 to 8182 (apache#3335) Corrects the default metrics port from 8282 to 8182 and clarifies the available metrics endpoints in the documentation. * fix(deps): update dependency software.amazon.awssdk:bom to v2.40.16 (apache#3336) * Add KMS options to catalogs create CLI (apache#3330) * Add Polaris blog about KMS (apache#3331) * Add Polaris blog about KMS Following up on apache#2802 * Use mainstream PostgreSQLContainer (apache#3345) Migrate from the deprecated PostgreSQLContainer class to the mainstream one as suggested by javadoc. * fix(deps): update dependency org.apache.iceberg:iceberg-bom to v1.10.1 (apache#3317) * Enforce mypy for CLI (apache#3305) * fix(deps): update dependency software.amazon.awssdk:bom to v2.41.1 (apache#3351) * Last merged commit e75eb4b --------- Co-authored-by: Yong Zheng <[email protected]> Co-authored-by: Pierre Laporte <[email protected]> Co-authored-by: Tamas Mate <[email protected]> Co-authored-by: Mend Renovate <[email protected]> Co-authored-by: rishii-19-works <[email protected]> Co-authored-by: Dmitri Bourlatchkov <[email protected]>
4 participants
Following up on #2802
Checklist
CHANGELOG.md(if needed)site/content/in-dev/unreleased(if needed)